fix(sync): notification can get stuck. - #20363

Logically seems correct to me but can you add tests to verify this, add tests to see if you can reproduce the error there and your solution later should pass the tests.

Added Robolectric test for SyncMediaWorker that simulates an error after the foreground notification is active and verifies the notification is always cancelled from the finally block.

When we encounter an error in the workers we are currently showing a notification but your code would immediately cancel this notification.

I'm also less inclined to accept a fix if there's no reproduction case.

To avoid the error notification being cancelled, can we use separate notification IDs for failed cases so the finally block only cancels the progress notification?? @lukstbit

I’ve tried multiple times to reproduce the original stuck-notification bug, but I haven’t been able to so far. The added test covers the same scenario (error after the notification is shown) and it passes with the fix, i'll keep testing on a real device and if I’m able to reproduce it i'll share a video here.

I don't think different notifications ids would fix this, feels something related to the WorkManager system itself when the worker is cancelled(maybe due to the constraint). I propose to just add a call to notificationManager?.cancel(NotificationId.SYNC_MEDIA) just before the cancelMediaSync(CollectionManager.getBackend()) line in the catch that deals with the coroutine cancellation and see how this goes in production.

The test should be removed, it mocks so much that it isn't real life behavior.

Got it, thanks for the explanation and suggestion.

It'd be great to see this fixed!

Drafting this until it's ready.

Please mark it as ready for review once a reproduction case is found (even if this is just a patch to the code).

In addition:

• Consider more logging if it's currently insufficient.
• Figure out why 'cancel'/swiping the notification wasn't working, and potentially also fix this failure case.

It'd be great to see this fixed!

Drafting this until it's ready.

Please mark it as ready for review once a reproduction case is found (even if this is just a patch to the code).

In addition:

• Consider more logging if it's currently insufficient.
• Figure out why 'cancel'/swiping the notification wasn't working, and potentially also fix this failure case.

@david-allison, I still haven’t been able to reproduce the stuck-notification behavior naturally on my device, but the stuck notification is likely caused by WorkManager cancelling the sync worker due to constraints, as lukstbit mentioned also. (network drop, battery saver, OEM killing the app etc.)

When this happens, the worker throws a CancellationException. We abort the backend sync, but earlier we did not cancel the sync notification in that flow. As a result, the ongoing “Syncing…” notification could remain visible and could not be swiped away.

The fix adds an explicit notificationManager.cancel(NotificationId.SYNC / SYNC_MEDIA) inside the CancellationException handler, so when WorkManager cancels the job, the progress notification is properly cleared instead of getting stuck.

Hello 👋, this PR has had no activity for more than 2 weeks and needs a reply from the author. If you think this is a mistake please comment and ping a maintainer to get this merged ASAP! Thanks for contributing! You have 7 days until this gets closed automatically

thanks so much!!

@david-allison The test you added is failing and it doesn't look right, you're throwing an exception on a method that is called before the call you are checking for.

The test should be removed, it mocks so much that it isn't real life behavior.

@lukstbit I believe I re-added the test in my force push (given the above message), apologies. If this isn't the same test, please flag it, I do not recall writing it.

@Giyutomioka-SS can you clarify which parts of the issue this pr doesn't solve? or should #19830 be marked as fixed?

@LUwUcifer, this PR covers the normal sync completion/error paths but doesn’t fully handle the WorkManager worker cancellation path yet, so that’s where the notification could still get stuck.

I couldn’t reproduce this, could you try and see if it happens on your side?
If yes, please share the steps, device/Android version, and logs here.

Nope, should add "can't reproduce" tag to #19830